[Fix](pyudf) Fix some pyudf issues#62249
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
25ea1db to
1f85b7a
Compare
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
fix p0 fix p0 fix p0
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found blocking correctness issues in the PR.
Critical checkpoint conclusions:
- Goal/test coverage: the PR targets Python UDF/UDAF/UDTF empty-argument support, deterministic metadata, error propagation, process recovery, and cache cleanup. It adds focused regression/unit coverage, but the implementation still misses BE enforcement for Python nondeterminism and produces non-recreatable SHOW CREATE FUNCTION SQL for Python functions.
- Scope/focus: the change is not as small as possible; several production INFO logs with
[pyudf-test]remain in hot analysis/execution paths. - Concurrency/lifecycle: the new UDAF cache broadcast runs from an agent task worker and can fail when a Python process dies between liveness check and Flight action; the new
voidwrapper converts that recoverable failure into an uncaught exception risk. - Configuration/incompatibility: no config or storage-format changes found.
- Parallel code paths: Python UDF/UDAF/UDTF deterministic metadata is passed through FE/Nereids, but the BE execution path still has no deterministic flag, so constant evaluation can still treat nondeterministic Python UDFs as constants.
- Conditional checks/error handling: Python UDAF error propagation is improved, but the new cleanup wrapper violates the repository error-handling pattern by throwing from a task callback instead of returning/logging
Status. - Test coverage: positive tests were added, but they do not cover BE constantness for nondeterministic zero-arg Python UDFs or re-executing SHOW CREATE FUNCTION output for Python inline/module UDF/UDAF/UDTF.
- Observability: existing diagnostics are sufficient for the reported runtime paths, but the leftover
[pyudf-test]INFO logs should be removed or demoted before merge. - Transaction/persistence/data correctness: no transaction/data visibility path changes found.
- User focus: no additional user-provided review focus was present.
| &input_batch, _timezone_obj)); | ||
| if (arguments.empty()) { | ||
| input_batch = arrow::RecordBatch::Make(schema, input_rows, | ||
| std::vector<std::shared_ptr<arrow::Array>> {}); |
There was a problem hiding this comment.
This adds zero-argument Python UDF execution, but BE still has no Python deterministic metadata. VectorizedFnCall::is_constant() only special-cases zero-arg JAVA_UDF, so a zero-arg Python UDF declared deterministic="false" can still be folded/evaluated once for a fragment and reused for all rows. A function like return random.random() or return time.time() would return one repeated value in SELECT py_rand() FROM t, despite the new FE/Nereids deterministic flag. Please pass/enforce the deterministic flag in the BE function constantness path, or at least disable constant treatment for zero-arg Python UDFs as Java UDFs do.
| if (clean_req.__isset.function_id && clean_req.function_id > 0) { | ||
| UserFunctionCache::instance()->drop_function_cache(clean_req.function_id); | ||
| PythonServerManager::instance().clear_udaf_state_cache(clean_req.function_id); | ||
| } |
There was a problem hiding this comment.
clear_udaf_state_cache() is a void wrapper that uses THROW_IF_ERROR around a broadcast to Python processes. This callback does not catch exceptions, so a normal race where a Python process dies after is_alive() but before FlightClient::Connect()/DoAction() can make the clean-cache agent task throw out of the worker instead of just logging the failed cleanup. The neighboring module-cache cleanup already returns Status and logs failures; please make the UDAF cleanup follow that pattern instead of throwing from this task callback.
| boolean isReturnNull = fn.getNullableMode() == NullableMode.ALWAYS_NULLABLE; | ||
| sb.append(",\n \"ALWAYS_NULLABLE\"=").append("\"" + isReturnNull + "\""); | ||
| sb.append(",\n \"DETERMINISTIC\"=").append("\"" + fn.isDeterministic() + "\""); | ||
| } else { |
There was a problem hiding this comment.
The Python branch still does not emit all properties required to recreate the function. CreateFunctionCommand requires runtime_version for every PYTHON_UDF, and inline Python functions also need the persisted functionCode rendered as AS $$...$$; with this output, SHOW CREATE FUNCTION followed by executing the shown SQL fails with Python runtime version is not set or recreates a module function without the inline body. For Python UDTFs, the same scalar path also prints CREATE FUNCTION ... RETURNS <item type> instead of CREATE TABLES FUNCTION ... RETURNS ARRAY<...>, losing table-function semantics. Please make the generated SQL round-trip for Python UDF/UDAF/UDTF before adding only the deterministic property.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)